[pull] master from DataDog:master#572
Merged
Merged
Conversation
…efresh the command UX (#23859) * Migrate `create` from datadog_checks_dev to ddev and revamp the UX Port the `create` command into ddev as a click group with one subcommand per integration type (`check`, `check-only`, `jmx`, `logs`, `event`, `metrics-crawler`). The `tile`, `snmp_tile`, and `marketplace` templates are not exposed as subcommands but their files stay on disk under datadog_checks_dev/ (in-toto rule). UX changes bundled into the migration: - Manifest-less is the default. `--include-manifest` opts into the legacy manifest-shipped path. - `--display-name`, `--metrics-prefix`, `--platforms` are first-class options. Missing values prompt in TTY and abort listing every missing flag in non-TTY. - `--non-interactive` removed. `--skip-manifest` accepted as a no-op with a deprecation warning; conflicts with `--include-manifest`. - `--type` accepted as a deprecation shim; aborts for dropped types pointing at the Building Integrations Without a Manifest Confluence page. - Manifest-less integrations get three entries written into `.ddev/config.toml` under `[overrides.display-name]`, `[overrides.metrics-prefix]`, and `[overrides.manifest.platforms]`. Templates are copied into ddev's package data; pytest, ruff, and mypy configs exclude the templates tree. * Add changelog * Fix check_only target directory when integration name has an author prefix `ddev create check-only partner_thing` was scaffolding into `root/thing/` instead of populating the existing `root/partner_thing/` directory that holds the manifest. The scaffold helper now distinguishes the on-disk integration directory (`target_integration_dir`) from the template ``{check_name}`` substitution (`check_name_override`). Other changes: - Replace `click.echo(..., err=True)` and `ctx.fail(...)` in the ``--type`` deprecation shim with `app.display_warning` / `app.abort` so the messages flow through ddev's color and verbosity layer. - Honour `app.interactive` (and therefore the global `--interactive/--no-interactive` flag) instead of probing `sys.stdin.isatty()` directly. - Surface a clear, subcommand-pointing error when a user runs the legacy `ddev create NAME` shape with no `--type`. - Update `docs/developer/tutorials/logs/http-crawler.md` to the new `ddev create logs ACME …` form. - Extract a `create_options` decorator factory in `_common.py`; the six per-type subcommand modules now share one option contract. - Wrap template rendering with template-path context on format errors, and replace `assert isinstance(...)` in `TemplateFile.write()` with an explicit runtime check. - On a per-file write failure, abort with a "wrote N/M files, remove `<dir>` and retry" message instead of leaving the user guessing. - Probe `.ddev/config.toml` readability before scaffolding and emit a hand-edit fallback if the post-scaffold override write fails. - Tighten the `--type` shim's short-flag matching to an allow-list of known integration types so future `-tXxx` flags can't be eaten. - Drop the unused `include_manifest` parameter from `_resolve_check_only_inputs`; flatten the dead-code guard in `run_subcommand`. - New tests cover the check_only directory regression, the manifest-less check_only overrides path, the bare-positional error, and the global `--no-interactive` flag's effect on `create`. * Fix directory connector in dry-run tree at depth >= 2 `_format_line` was using `PIPE_END if last or is_dir else PIPE_MIDDLE` for nodes at depth >= 2, so every directory rendered with `└──` regardless of its position among siblings. Drop the `is_dir` short-circuit so non-last directories use `├──` like every other non-last node. Other changes: - Restructure `run_subcommand` into two explicit paths (manifest-less vs `--include-manifest`) so the manifest-less locals' scope matches their use; the override write moves into its own helper. - Remove the unreferenced `CHECK_ONLY_LINKS` constant and its entry in `INTEGRATION_TYPE_LINKS`. The `check_only` branch in `construct_template_fields` never consulted the dict. - Distinguish "no `--type` flag" from "`--type` with no value" via a dedicated `_MissingTypeValue` sentinel; the latter now aborts with a targeted message naming the missing value. - Wrap the `manifest.json` read in `_resolve_check_only_inputs` with `try/except (OSError, json.JSONDecodeError)` and route through `app.abort` for parity with the surrounding aborts. - Add a `read_config` fixture that loads `.ddev/config.toml` and switch the test module from the gated `tomli` to stdlib `tomllib` (Python 3.13). Deduplicate the three tests that previously inlined the parse-and-assert dance. - New tests for the tree-connector regression and the `--type`-with- no-value abort. * Fix JMX template config_models so scaffolded integrations don't crash at runtime Every `defaults` function in the JMX template was defined `(field, value)` while `instance.py` invoked them with zero arguments via `getattr(defaults, ..., lambda: value)()`. Any integration scaffolded with `ddev create jmx` raised `TypeError: ...() missing 2 required positional arguments` on first run. Drop the parameters to match every production JMX integration in the repo (e.g. `activemq`). The legacy template under `datadog_checks_dev/` stays byte-identical per the in-toto rule. Other changes: - `_extract_legacy_type` now treats a flag-shaped token following `--type` / `-t` (e.g. `ddev create NAME --type --dry-run`) as the flag's value being missing, so the user gets the targeted "requires a value" message instead of `Unknown integration type '--dry-run'`. - `construct_template_fields` switches from `INTEGRATION_TYPE_LINKS[...]` to `.get(..., '')` so a not-yet-registered subcommand doesn't raise an unattributed `KeyError` during scaffolding. - Tighten `_write_manifestless_overrides`'s `integration_dir` from `Any` to `Path`, and introduce a `CheckOnlyPrefillFields` `TypedDict` for the manifest-prefilled fields produced by `prefill_check_only_fields`. - Guard `_resolve_check_only_inputs` against non-object JSON manifests with a clean `app.abort('... does not contain a JSON object')`. - Document the `TemplateFile` invariant (`binary=True` => `bytes`, `binary=False` => `str`) so the two `# type: ignore` lines read as intentional. - `_write_files_with_cleanup_hint` now branches by `integration_type`: for `check_only`, list the scaffolded files (so the user knows what to remove without touching their `manifest.json`); for everything else, the existing "Remove `<integration_dir>` and retry" message stands. - Drop the unreachable `check_name_override` parameter from `render()` and the corresponding third return value of `_resolve_check_only_inputs`. `prefill_check_only_fields` already populates `check_name`, making the guard dead code. - Update `docs/developer/tutorials/jmx/integration.md` to the new `ddev create jmx NAME ...` form to match the round-1 update for the logs tutorial. * Reword test docstrings to describe behaviour and fit the line limit Several test docstrings in ddev/tests/cli/create/test_create.py described their own review-history provenance instead of the behaviour they lock in, and one of them was 131 characters wide which tripped ruff E501 on CI. Reword every affected docstring so each test reads as a self-contained behavioural assertion and stays under 120 chars. * Restore JMX metrics.py template to match legacy ground truth The new copy of `jmx/{check_name}/tests/metrics.py` had been cosmetically reformatted by ruff (multi-line list/concat layout) during an early implementation pass — before the templates tree was added to the ruff format/lint excludes. The two forms are functionally identical, but the new template now scaffolded a visibly different metrics.py than legacy `ddev create jmx` would have produced. Revert the file to the master legacy bytes so the two are byte-identical again. The ruff configs in `ddev/pyproject.toml` (`[tool.ruff].exclude`, `[tool.ruff.format].exclude`) and the root `pyproject.toml` already list `src/ddev/cli/create/templates`. Verified with `ddev test --lint ddev`: 303 files already formatted, no rewrite of the restored metrics.py. * Reject empty author name when scaffolding check-only integrations `_resolve_check_only_inputs` checked `if author is None` against the value pulled from `manifest_data["author"]["name"]`. An empty string (or whitespace-only string) passed that guard and propagated through `prefill_check_only_fields` (which filtered the field out) and `construct_template_fields` (which fell back to `check_name = ''`). The rendered template paths then collapsed to forms like `/datadog_checks//__about__.py`, and `Path(root) / '/abs/path'` discards `root` on POSIX — scaffolded files would have been written to the filesystem root. Switch to a truthy check on the `.strip()`-ed value so empty and whitespace-only author names abort cleanly. Other changes: - Replace the 13-line `_MissingTypeValue` singleton class with a bare module-level `_MISSING_TYPE_VALUE = object()`. The only consumer uses `is`-comparison, not `isinstance`, so the class machinery added nothing. - Extract `_TYPE_FLAG_LITERALS` and `_TYPE_FLAG_EQUALS_PREFIXES` module constants shared by `_extract_legacy_type` and `_strip_type_flag` so future spellings only have to be added in one place. * Reject all-symbol author names and surface invalid integration names cleanly `_resolve_check_only_inputs` previously accepted any truthy author name after `.strip()`. An all-symbol input like "!@#$" passed the guard but then `normalize_display_name` collapsed it to "", and the downstream `removeprefix` was a no-op — leaving the rendered template paths with a stray leading underscore. Consolidate the normalization and the truthy check: normalize first, abort when the normalized form is empty. Same guard now covers empty, whitespace-only, and all-symbol author names. Other changes: - Pre-validate the integration `name` argument in `run_subcommand` via a shared `is_valid_integration_name` predicate in `_naming.py`. Names with leading/trailing non-alphanumerics or characters outside `[A-Za-z0-9._\- ]` now abort with a clear message instead of surfacing a raw `ValueError` from `normalize_project_name`. - Swap the bare `_MISSING_TYPE_VALUE = object()` sentinel for a one-member `Enum` (`_TypeFlagSentinel.MISSING`). Same runtime semantics (identity comparison), distinct nominal type so mypy can narrow `_extract_legacy_type`'s return value. - Drop the per-subcommand `-q`/`--quiet` flag from `create_options`. The root `ddev` group already exposes a count-based `--quiet`/`--verbose`; `render()` now reads `app.quiet` and routes the one-line headline through `app.display` (unconditional) so `ddev -q create ...` still prints "Created `<dir>`". - Extract a `fail_on_second_write` pytest fixture in `conftest.py` and drop the duplicated `monkeypatch.setattr(TemplateFile.write, ...)` setup from the two partial-write tests. * Fix package name collision when manifest author contains a hyphen `_resolve_check_only_inputs` stripped the author prefix from the target integration directory using `normalize_display_name`, which preserves hyphens. The directory name is built via `normalize_package_name`, which converts hyphens to underscores. A manifest with `"author": {"name": "My-Partner"}` paired with a directory named `my_partner_thing` produced an `author_normalized = "my-partner"` prefix that no longer matched the underscore form in the dir. The `removeprefix` silently did nothing, and `prefill_check_only_fields` then re-prefixed the (unstripped) name with the same author segment, yielding a doubled `my_partner_my_partner_thing` Python package path. Use `normalize_package_name(author_normalized)` so the prefix matches the directory's normalization. Other changes: - Reword `ddev/changelog.d/23859.added` to describe the genuinely new user-facing surface (subcommand-based interface plus the new options) instead of the migration-implementation detail; the migration narrative stays in `23859.changed`. - Add behaviour tests for `--location` with both absolute and relative target paths, plus an interactive-prompt test that exercises the empty-input default-acceptance branch of `_resolve_manifestless_inputs`. - Add a comment at `TemplateFile.read()` documenting that template rendering failures indicate a broken shipped template, not user error — the user's `name` is validated upstream by `is_valid_integration_name` before scaffolding starts. * Show TOML section headers in the create config-write-failure message The manual-fix instructions printed when `.ddev/config.toml` can't be written listed the override values without their `[overrides.*]` section headers: Rich parsed the bracketed headers as style tags and stripped them. Pass markup=False so the headers survive. Also hardens the surrounding scaffolding: - Treat `--type=` with an empty value as a missing type, not the type name ``. - Derive check_class from the final check_name and assert it is populated before scaffolding, so an empty name can't collapse paths to the filesystem root. - Reuse the caller's already-normalized author in prefill_check_only_fields instead of re-deriving it from the raw manifest. - Narrow the override-setter type hints. - Add tests for the malformed-config and config-write-failure abort paths. * Test that create rejects --type= with an empty value * Accept the legacy --type flag before the positional name The deprecation shim resolved --type inside the group's command resolution, which click only reaches after its option parser runs. When --type appeared before the name (the form the pre-migration docs used, e.g. `ddev create --type jmx NAME`), the parser rejected it with "No such option" before the shim could dispatch. Setting ignore_unknown_options on the group lets the flag reach the shim in any position; subcommand option typos are still rejected because that setting does not propagate to the resolved subcommand. --------- Co-authored-by: Luis Orofino <luis.orofino@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )